-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Stablize CARGO_RUSTC_CURRENT_DIR
#13644
base: master
Are you sure you want to change the base?
Conversation
This provides what cargo sets as the `current_dir` for the `rustc` process. While `std::file!` is unspecified in what it is relative to, it is relatively safe, it is generally relative to `rustc`s `current_dir`. This can be useful for snapshot testing. For example, `snapbox` has been using this macro on nightly since assert-rs/snapbox#247, falling back to finding a parent of `CARGO_MANIFEST_DIR`, if present. This has been in use in Cargo since rust-lang#13441. This was added in rust-lang#12996. Relevant points discussed in that issue: - This diverged from the original proposal from the Cargo team of having a `CARGO_WORKSPACE_DIR` that is the "workspace" of the package being built (ie registry packages would map to `CARGO_MANIFEST_DIR`). In looking at the `std::file!` use case, `CARGO_MANIFEST_DIR`, no matter how we defined it, would only sort of work because no sane definition of that maps to `rustc`'s `current_dir`.a This instead focuses on the mechanism currently being used. - Using "current dir" in the name is meant to be consistent with `std::env::current_dir`. - I can go either way on `CARGO_RUSTC` vs `RUSTC`. Existing related variables: - `RUSTC` - `RUSTC_WRAPPER` - `RUSTC_WORKSPACE_WRAPPER` - `RUSTFLAGS` (no `C`) - `CARGO_CACHE_RUSTC_INFO` Note that rust-lang#3946 was overly broad and covered many use cases. One of those was for packages to look up information on their dependents. Issue rust-lang#13484 is being left open to track that. Fixes rust-lang#3946
r? @weihanglo rustbot has assigned @weihanglo. Use |
@rfcbot fcp merge |
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
base.get_cwd() | ||
.map(|c| c.display().to_string()) | ||
.unwrap_or(String::new()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if it could just be.
base.get_cwd() | |
.map(|c| c.display().to_string()) | |
.unwrap_or(String::new()), | |
base.get_cwd().unwrap_or_default(), |
(and ditto for CARGO_TARGET_TMPDIR
? though that is not relevant to this PR)
@@ -265,7 +265,7 @@ corresponding environment variable is set to the empty string, `""`. | |||
where integration tests or benchmarks are free to put any data needed by | |||
the tests/benches. Cargo initially creates this directory but doesn't | |||
manage its content in any way, this is the responsibility of the test code. | |||
* `CARGO_RUSTC_CURRENT_DIR` --- This is a path that `rustc` is invoked from **(nightly only)**. | |||
* `CARGO_RUSTC_CURRENT_DIR` --- This is a path that `rustc` is invoked from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still feel like people might overlook this, as it isn't an obvious thing easy to relate to their problems. That said, I am fine with the previous explanation. Just forward it here if people have the same question: #12996 (comment).
When we talked about this, Eric was hesitant about locking down the behavior of
file!
so I left out that use case. It should work and I can't imagine rustc doing anything but absolute or relative tostd::env::current_dir
but I'm leaving that to people to make the choice that they want to make those assumptions, rather than us advertising it.
Is there some information about the use cases for this? How is it expected to be used? |
One of the requests in #3946 is a way to be able to resolve For example, snapbox::path::current_dir!) is useful for
Possible workarounds
As you mentioned the last time the Cargo team discussed this, For a summary of the team's last discussion on this, see #3946 (comment) |
I am concerned that if the primary (or only?) way to use this environment variable is with Were there other considerations made, such as adding something like Have there been any discussions with the libs team, and what their thoughts are on making
Is this limited to only test/bench? |
This provides what cargo sets as the
current_dir
for therustc
process.While
std::file!
is unspecified in what it is relative to, it is relatively safe, it is generally relative torustc
scurrent_dir
.This can be useful for snapshot testing.
For example,
snapbox
has been using this macro on nightly since assert-rs/snapbox#247, falling back to finding a parent ofCARGO_MANIFEST_DIR
, if present.This has been in use in Cargo since #13441.
This was added in #12996.
Relevant points discussed in that issue:
CARGO_WORKSPACE_DIR
that is the "workspace" of the package being built (ie registry packages would map toCARGO_MANIFEST_DIR
). In looking at thestd::file!
use case,CARGO_MANIFEST_DIR
, no matter how we defined it, would only sort of work because no sane definition of that maps torustc
'scurrent_dir
.a This instead focuses on the mechanism currently being used.std::env::current_dir
.CARGO_RUSTC
vsRUSTC
. Existing related variables:RUSTC
RUSTC_WRAPPER
RUSTC_WORKSPACE_WRAPPER
RUSTFLAGS
(noC
)CARGO_CACHE_RUSTC_INFO
Note that #3946 was overly broad and covered many use cases. One of those was for packages to look up information on their dependents.
Issue #13484 is being left open to track that.
Fixes #3946